Skip to content

clp-bench: Refactor and Add New Results#5

Open
anlowee wants to merge 90 commits intoy-scope:mainfrom
anlowee:xiaochong-refactor-and-add-new-results
Open

clp-bench: Refactor and Add New Results#5
anlowee wants to merge 90 commits intoy-scope:mainfrom
anlowee:xiaochong-refactor-and-add-new-results

Conversation

@anlowee
Copy link
Contributor

@anlowee anlowee commented Nov 4, 2024

This PR does a major refactoring. The main change is that we lock the clp-bench executor's code logic, for each target tool, the user shall provide corresponding scripts that to execute tasks of benchmarking.

Benefits:

  • No need to modify the clp-bench package's code everytime we add a new target tool to benchmark against.
  • The scripts of each target tool are stored in a asset folder, which is clear and easy to manage.

@kirkrodrigues
Copy link
Member

Sorry, won't be able to review this until tonight.

@kirkrodrigues
Copy link
Member

Sorry, was sick over the weekend so didn't get a chance to look at this. Will review at least the docs today.

@anlowee
Copy link
Contributor Author

anlowee commented Nov 19, 2024

Sorry, was sick over the weekend so didn't get a chance to look at this. Will review at least the docs today.

Take care! It becomes very vold suddenly these days.

Copy link
Member

@kirkrodrigues kirkrodrigues left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some feedback on the docs. I would also like to clean up the directory structure, but I guess that may take you some time, so we can defer it to the next PR.

Copy link
Member

@kirkrodrigues kirkrodrigues left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some additional comments besides those inline:

  • For all Markdown link references (i.e., the b in [a][b]):
    • can we avoid using spaces?
    • can we keep them short (that's the goal of using link references)?

Thanks for making the changes requested so far. To give you some context, my goal is that we get this PR to a state where the docs and usage are clear. Then we can defer any additional refactoring to future PRs.

@anlowee anlowee requested a review from kirkrodrigues December 4, 2024 15:15
@kirkrodrigues
Copy link
Member

Sorry, need a little more time to finish reviewing this.

@kirkrodrigues
Copy link
Member

Is the first flowchart in the methodology being displayed as you expect?

@anlowee
Copy link
Contributor Author

anlowee commented Dec 9, 2024

Is the first flowchart in the methodology being displayed as you expect?

Apparently no lol, so weird. It should look like this:
image

Don't know why it gets rendered so weirdly on github

@anlowee
Copy link
Contributor Author

anlowee commented Jan 16, 2025

Is the first flowchart in the methodology being displayed as you expect?

Fixed this in the latest commit

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants